Skip to content

Rename resolveReference to resolve and add <T> List<T> getBeans(Class<T> clazz)#2492

Closed
christiangoerdes wants to merge 4 commits into
masterfrom
beanregistry-improvements
Closed

Rename resolveReference to resolve and add <T> List<T> getBeans(Class<T> clazz)#2492
christiangoerdes wants to merge 4 commits into
masterfrom
beanregistry-improvements

Conversation

@christiangoerdes

@christiangoerdes christiangoerdes commented Dec 22, 2025

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features

    • Asynchronous bean collection for configuration events, improving responsiveness during dynamic updates.
    • Improved YAML parsing and Kubernetes watcher integration for more reliable configuration ingestion.
  • Chores

    • Reorganized bean registry and lifecycle plumbing for better modularity and maintainability.
    • Added convenience helpers for change-action checks to simplify configuration-state handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Dec 22, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

A refactor relocates the YAML/bean registry surface into a new beanregistry package, renames and simplifies several APIs (e.g., resolveReferenceresolve, getBeans() generified), introduces BeanDefinitionChanged and BeanContainer, adds an async wrapper AsyncBeanCollector, and replaces the prior two-step registry startup with parseYamls(). Imports and call sites across core and tests were updated accordingly.

Changes

Cohort / File(s) Summary
Build
annot/pom.xml
Added dependency com.github.spotbugs:spotbugs-annotations:4.9.8.
Core interfaces & package move
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java, annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java
Moved to beanregistry package. resolveReference(String)resolve(String). getBeans() → generic <T> List<T> getBeans(Class<T>). Removed registerBeanDefinitions() and start(). ChangeEvent made public sealed.
Bean definition model
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java, .../BeanDefinitionChanged.java, .../BeanContainer.java
BeanDefinition simplified (holds only JsonNode); factory now returns BeanDefinitionChanged. New public record BeanDefinitionChanged(action, bd) and new BeanContainer holding definition + volatile singleton.
Collector API & async wrapper
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java, .../AsyncBeanCollector.java
New BeanCollector interface with default parseYamls(...), handle(ChangeEvent, boolean), and start(). AsyncBeanCollector buffers events and delegates processing on a background thread.
Registry implementation (new package)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
New implementation implementing BeanRegistry and BeanCollector: manages containers, activation queue, singleton/prototype resolution, and observer notifications.
Removed legacy implementation
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java
Old yaml-package registry implementation removed.
Reference resolution & parsing updates
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java, annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java, .../MethodSetter.java, .../ParsingContext.java
Calls updated from registry.resolveReference(...)registry.resolve(...). Imports updated to new package locations; docs adjusted.
Observer API change
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java
handleBeanEvent(...) parameter type changed from BeanDefinitionBeanDefinitionChanged.
WatchAction helpers
annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java
Added isDeleted(), isModified(), isAdded() predicate methods.
Core integrations
core/.../Router.java, core/.../kubernetes/KubernetesWatcher.java, core/.../cli/RouterCLI.java, core/.../config/spring/k8s/Envelope.java
Updated imports and call sites to new types. Router.handleBeanEvent now accepts BeanDefinitionChanged and checks bdc.action().is*(); KubernetesWatcher wires BeanRegistryImplementation into an AsyncBeanCollector and invokes handle(...) with BeanDefinition. RouterCLI switched from BeanRegistry to BeanCollector.parseYamls().
Tests & test utils
annot/src/test/**, core/src/test/** (e.g., YAMLBeanParsingTest.java, GenericYamlParserTest.java, CompilerHelper.java, InMemoryClassLoader.java)
Imports and overrides updated for package move and method rename (resolveReferenceresolve); classloader delegation updated to reference new BeanRegistry class.
Test util YAML init
annot/src/test/java/.../YamlParser.java
Replaced registerBeanDefinitions() + start() flow with BeanRegistryImplementation impl; impl.parseYamls(...); and observer callback signature updated to BeanDefinitionChanged.

Sequence Diagram(s)

mermaid
sequenceDiagram
autonumber
participant K as KubernetesWatcher
participant A as AsyncBeanCollector
participant R as BeanRegistryImplementation
participant O as BeanCacheObserver

K->>A: handle(BeanDefinitionChanged event, true)
note right of A #DFF2E1: enqueue event\non background thread
A-->>R: (background thread) handle(event, isLast)
R->>R: activationRun (process queued containers)
R->>O: handleBeanEvent(BeanDefinitionChanged, bean, oldBean)
note right of O #FFF4D6: observer processes registration/removal

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Possibly related PRs

Suggested labels

7.x

Suggested reviewers

  • predic8
  • rrayst
  • t-burch

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the two main API changes in the PR: renaming resolveReference to resolve and adding a generic getBeans method.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch beanregistry-improvements

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d82b45b and 8aea5e8.

📒 Files selected for processing (2)
  • annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java
  • annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (java)
🔇 Additional comments (5)
annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java (1)

18-18: LGTM! Import updated for package relocation.

The import correctly reflects the relocation of BeanRegistry to the beanregistry package.

annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (4)

17-17: LGTM! Import updated for package relocation.

The import correctly reflects the relocation of BeanRegistry to the beanregistry package.


80-81: LGTM! Method rename applied correctly.

Both calls correctly use the renamed resolve method. The shorter method name improves API clarity.


96-97: LGTM! Method rename applied correctly.

Both calls correctly use the renamed resolve method, consistent with the API changes.


111-111: LGTM! Method rename applied correctly.

The error-handling test correctly uses the renamed resolve method.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@christiangoerdes

Copy link
Copy Markdown
Collaborator Author

Merge #2488 first

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (6)
annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (1)

70-84: LGTM!

Test correctly uses the renamed resolve method. The test logic for singleton verification is sound.

Consider renaming the test method from singletonResolveReference to singletonResolve to match the new API naming.

annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)

18-18: Consider explicit imports over wildcard.

Wildcard imports can obscure dependencies. Consider explicit imports for clarity:

import com.predic8.membrane.annot.beanregistry.BeanCacheObserver;
import com.predic8.membrane.annot.beanregistry.BeanDefinition;
import com.predic8.membrane.annot.beanregistry.BeanDefinitionChanged;
import com.predic8.membrane.annot.beanregistry.BeanRegistry;
import com.predic8.membrane.annot.beanregistry.BeanRegistryImplementation;
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)

17-19: Remove unused import.

BeanRegistryImplementation is imported but not used in this interface. Consider removing it to keep the imports clean.

🔎 Proposed fix
 import com.predic8.membrane.annot.beanregistry.BeanDefinition;
 import com.predic8.membrane.annot.beanregistry.BeanDefinitionChanged;
-import com.predic8.membrane.annot.beanregistry.BeanRegistryImplementation;
annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java (1)

22-25: Consider documenting that isLast is intentionally ignored.

The isLast parameter is ignored because the async wrapper recalculates it based on queue state. A brief comment would clarify this design decision.

     @Override
     public void handle(ChangeEvent changeEvent, boolean isLast) {
+        // isLast is intentionally ignored; recalculated based on queue state in worker thread
         changeEvents.add(changeEvent);
     }
annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java (1)

17-19: Unused imports.

WatchAction and its static import are not used in this file. They may be leftover from refactoring or intended for BeanDefinitionChanged which is defined in a separate file.

🔎 Proposed fix
 package com.predic8.membrane.annot.beanregistry;
 
-import com.predic8.membrane.annot.yaml.WatchAction;
-
-import static com.predic8.membrane.annot.yaml.WatchAction.*;
-
 public sealed interface ChangeEvent permits BeanDefinitionChanged, StaticConfigurationLoaded {
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)

17-19: Unused import: BeanRegistry.

BeanRegistry is imported but not used in this file. Only BeanCollector and BeanRegistryImplementation are referenced.

🔎 Proposed fix
 import com.predic8.membrane.annot.beanregistry.BeanCollector;
-import com.predic8.membrane.annot.beanregistry.BeanRegistry;
 import com.predic8.membrane.annot.beanregistry.BeanRegistryImplementation;
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f05e64e and d82b45b.

📒 Files selected for processing (28)
  • annot/pom.xml
  • annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionChanged.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
  • annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java
  • annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java
  • annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java
  • annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java
  • annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java
  • annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java
  • annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java
  • annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java
  • core/src/main/java/com/predic8/membrane/core/Router.java
  • core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
  • core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java
  • core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java
  • core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java
  • core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java
💤 Files with no reviewable changes (1)
  • annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-19T09:01:40.311Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2441
File: annot/pom.xml:0-0
Timestamp: 2025-12-19T09:01:40.311Z
Learning: In annot/pom.xml, ensure log4j dependencies log4j-slf4j2-impl and log4j-core have <scope>test</scope>. In the distribution module (separate pom.xml), these dependencies should use the normal compile-scope. This guidance applies to all pom.xml files under the annot and distribution module structure where these dependencies appear; verify both modules are configured accordingly to reflect testing vs. runtime usage.

Applied to files:

  • annot/pom.xml
📚 Learning: 2025-09-22T19:49:29.473Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: pom.xml:0-0
Timestamp: 2025-09-22T19:49:29.473Z
Learning: In the Membrane API Gateway project, jackson-annotations is pinned to version "2.20" instead of using ${jackson.version} (2.20.0) because version 2.20.0 is not available in Maven repository yet for jackson-annotations, while it is available for jackson-core and jackson-databind. This version inconsistency is intentional and necessary due to different release schedules between Jackson modules.

Applied to files:

  • core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2240
File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215
Timestamp: 2025-10-23T13:04:11.388Z
Learning: In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision.

Applied to files:

  • annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java
🧬 Code graph analysis (5)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
  • McYamlIntrospector (28-185)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
  • BeanDefinition (21-103)
core/src/main/java/com/predic8/membrane/core/Router.java (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
  • BeanDefinition (21-103)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
  • BeanRegistryImplementation (29-176)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (2)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (1)
  • BeanDefinition (21-103)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
  • BeanRegistryImplementation (29-176)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (java)
🔇 Additional comments (45)
core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java (1)

27-27: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)

17-17: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (1)

18-18: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java (1)

19-19: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)

16-16: LGTM! Import improved from wildcard to specific import.

The change from wildcard import to specific import is better practice and correctly reflects the BeanRegistry package relocation.

annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java (1)

18-18: LGTM! Import path updated to reflect package relocation.

The BeanRegistry import has been correctly updated to use the new beanregistry package.

annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (2)

107-107: LGTM! API method renamed from resolveReference to resolve.

The call has been correctly updated to use the renamed method. This change aligns with the PR objectives to simplify the BeanRegistry API.


130-146: LGTM! API method renamed from resolveReference to resolve.

The call at Line 134 has been correctly updated to use the renamed method. The error handling and type checking logic remain unchanged.

annot/pom.xml (1)

59-63: Approved. SpotBugs annotations added for static analysis support.

Version 4.9.8 is current and officially released. The compile scope is correct for annotations used in code documentation.

annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java (1)

111-115: LGTM!

The delegation check correctly references the new package location for BeanRegistry (com.predic8.membrane.annot.beanregistry.BeanRegistry), ensuring the test class loader delegates to the parent for this interface.

annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (2)

18-18: LGTM!

Import correctly references the relocated BeanRegistry interface.


240-251: LGTM!

The method rename from resolveReference to resolve is applied correctly. The null-safety check and type-compatibility validation logic remain intact.

core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (2)

18-19: LGTM!

Imports correctly updated to reference BeanDefinition and BeanRegistry from the new beanregistry package.


335-359: LGTM!

TestRegistry correctly implements the renamed resolve method. The stub implementations for other BeanRegistry methods are appropriate for these parameterized tests that only exercise reference resolution.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinitionChanged.java (1)

1-11: LGTM!

Well-designed record for signaling bean definition changes. The pairing of WatchAction and BeanDefinition clearly captures the event semantics.

Consider adding null validation if action or bd should never be null at construction time:

public record BeanDefinitionChanged(
        WatchAction action,
        BeanDefinition bd) implements ChangeEvent {
    public BeanDefinitionChanged {
        Objects.requireNonNull(action, "action must not be null");
        Objects.requireNonNull(bd, "bd must not be null");
    }
}
annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java (1)

16-30: LGTM!

The predicate methods (isDeleted(), isModified(), isAdded()) provide a more readable API at call sites compared to direct enum comparisons.

annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (3)

17-17: LGTM!

Import correctly updated to the new beanregistry package.


86-100: LGTM!

Test correctly uses the renamed resolve method for prototype scope verification.


102-113: LGTM!

Error handling test correctly uses the renamed resolve method.

annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (4)

23-24: LGTM!

Imports correctly reference BeanDefinition and BeanRegistry from the new beanregistry package.


92-103: LGTM!

Javadoc accurately updated to reflect that the method returns "list of parsed bean definitions" rather than a fully populated registry.


262-268: LGTM!

The resolve method call correctly replaces resolveReference, with existing error handling preserved.


293-297: LGTM!

The $ref handling path correctly uses the renamed resolve method.

annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (2)

51-56: LGTM!

The two-step initialization pattern is correctly implemented:

  1. Create BeanRegistryImplementation with observer and grammar
  2. Parse YAMLs via parseYamls
  3. Assign to the beanRegistry field (typed as BeanRegistry interface)

This allows the implementation-specific parseYamls method to be called before treating the registry through its interface.


84-88: LGTM!

The handleBeanEvent signature correctly updated to accept BeanDefinitionChanged bdc instead of BeanDefinition bd, aligning with the new event model that pairs action with definition.

core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (2)

107-117: Consider whether isLast=true is always appropriate for watch events.

The handle method is always called with isLast=true. This works correctly for individual Kubernetes watch events processed one at a time. If batch processing is ever introduced, this would need adjustment to properly signal the last event in a batch.


49-51: No action required. Router implements BeanCacheObserver, so the constructor call is type-safe and correct. The code at lines 49-51 passes router to BeanRegistryImplementation, which expects a BeanCacheObserver as its first parameter—this is valid.

Likely an incorrect or invalid review comment.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCollector.java (2)

22-29: LGTM!

The parseYamls default method correctly orchestrates YAML ingestion: parsing, emitting BeanDefinitionChanged events with proper isLast flags, signaling StaticConfigurationLoaded, and finally calling start(). The empty list case is handled correctly since StaticConfigurationLoaded is always emitted.


31-41: Clean interface design.

The abstract methods handle and start are well-documented and provide a clear contract for implementations. The separation between event handling and lifecycle management is appropriate.

annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)

47-47: API change from BeanDefinition to BeanDefinitionChanged is appropriate.

Using BeanDefinitionChanged provides the WatchAction context (ADDED/MODIFIED/DELETED) alongside the BeanDefinition, which is more informative for event handling. This aligns with the new event-driven architecture.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)

3-26: LGTM - simple and clean container class.

The design is straightforward. The volatile modifier on singleton ensures visibility across threads. If singleton initialization is performed in a single-threaded context (e.g., during bean registry startup) with only concurrent reads afterward, this is appropriate.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java (1)

20-28: Clean API refinement.

The changes improve the interface:

  • resolve is more concise than resolveReference while maintaining clarity.
  • The generic getBeans(Class<T> clazz) enables type-safe retrieval, complementing the existing getBeans() for unfiltered access.
  • Separation of concerns is improved by moving lifecycle methods (registerBeanDefinitions, start) to BeanCollector.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/ChangeEvent.java (1)

21-28: LGTM!

The sealed interface design cleanly separates configuration events. StaticConfigurationLoaded being package-private appropriately restricts its creation to internal components.

core/src/main/java/com/predic8/membrane/core/Router.java (2)

18-19: LGTM!

The imports correctly reflect the refactored types: BeanDefinition for the isActivatable method and BeanDefinitionChanged for the event-driven handleBeanEvent method.


654-677: LGTM!

The refactored method correctly uses BeanDefinitionChanged to encapsulate both the action type and bean definition. The pattern bdc.action().isAdded() cleanly separates concerns, and accessing the wrapped definition via bdc.bd() is straightforward.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (6)

50-63: LGTM!

The define method correctly dispatches between BeanFactory for explicit "bean" kinds and GenericYamlParser for other types. Exception wrapping is appropriate for propagation.


69-87: LGTM!

The handle method cleanly uses pattern matching for deconstruction. The flow correctly processes StaticConfigurationLoaded and BeanDefinitionChanged events, triggering activation when appropriate.


124-139: LGTM!

The resolve method correctly handles prototype vs singleton scope: prototypes are always instantiated fresh, while singletons are cached in the container. The exception message is clear when a reference is not found.


145-161: Inconsistent filtering between getBeans() and getBeans(Class<T>).

getBeans() filters out components via !bd.getDefinition().isComponent(), but getBeans(Class<T>) does not apply this filter. This may return component beans unexpectedly when filtering by class.

Verify if this inconsistency is intentional. If not:

🔎 Proposed fix to add component filtering
     @Override
     public <T> List<T> getBeans(Class<T> clazz) {
         return bcs.values().stream()
+                .filter(bd -> !bd.getDefinition().isComponent())
                 .map(BeanContainer::getSingleton)
                 .filter(Objects::nonNull)
                 .filter(clazz::isInstance)
                 .map(clazz::cast)
                 .toList();
     }

168-175: LGTM!

The prototype scope detection correctly differentiates between regular definitions (using isPrototype()) and explicit "bean" kinds (reading from JSON). Defaulting to SINGLETON is a sensible convention.


37-41: Thread-safety of uidsToActivate requires either explicit documentation or synchronization.

In production usage (KubernetesWatcher), BeanRegistryImplementation is wrapped with AsyncBeanCollector, which enforces thread confinement by processing all handle() calls from a single background thread. However, the code doesn't document this assumption, and direct usage without AsyncBeanCollector (as in RouterCLI and tests) could allow concurrent modification of the plain LinkedHashSet if called from multiple threads.

Consider either: (1) documenting that handle() must be called from a single thread, or (2) making uidsToActivate thread-safe (e.g., Collections.synchronizedSet() or adding synchronization) to prevent accidental misuse when the wrapper is omitted.

annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanDefinition.java (3)

14-19: LGTM!

Package relocation to beanregistry and the WatchAction import support the new factory method pattern.


34-50: LGTM!

The refactoring cleanly separates concerns: the private constructor handles K8S-specific JSON parsing, while create4Kubernetes returns a BeanDefinitionChanged that encapsulates both the action and definition. This aligns well with the event-driven architecture introduced in this PR.


52-102: LGTM!

The public constructor and accessor methods remain intact for non-Kubernetes use cases. The utility methods (isComponent, isBean, isPrototype) provide clean abstractions for scope and type determination.

core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)

172-173: Review parseYamls method signature for potential simplification.

The grammar parameter passed to parseYamls duplicates the instance field stored during construction. Both current usages pass identical grammar instances to the constructor and method. Consider whether parseYamls should use the stored instance grammar instead of requiring the parameter, or document if allowing different grammars per parse operation is intentional design.

Comment on lines +31 to +42
t = new Thread(() -> {
while (true) {
try {
ChangeEvent changeEvent = changeEvents.take();
delegate.handle(changeEvent, changeEvents.isEmpty());
} catch (InterruptedException e) {
break;
}
}
});
t.start();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Thread lifecycle and configuration issues.

Several concerns with the background thread:

  1. No shutdown mechanism: There's no stop() method to gracefully terminate the thread. Consider adding one that interrupts the thread and awaits termination.

  2. Non-daemon thread: The thread will prevent JVM shutdown. Consider setting t.setDaemon(true) or providing explicit shutdown.

  3. Thread naming: Add a descriptive name for debugging: t.setName("AsyncBeanCollector-worker").

  4. Exception handling: If delegate.handle() throws an unchecked exception, the thread will terminate silently.

🔎 Proposed improvements
     @Override
     public synchronized void start() {
         if (t != null)
             return;
         t = new Thread(() -> {
             while (true) {
                 try {
                     ChangeEvent changeEvent = changeEvents.take();
                     delegate.handle(changeEvent, changeEvents.isEmpty());
                 } catch (InterruptedException e) {
                     break;
+                } catch (Exception e) {
+                    // Log and continue processing
+                    e.printStackTrace();
                 }
             }
         });
+        t.setName("AsyncBeanCollector-worker");
+        t.setDaemon(true);
         t.start();
     }
+
+    public synchronized void stop() {
+        if (t != null) {
+            t.interrupt();
+            try {
+                t.join(5000);
+            } catch (InterruptedException ignored) {
+                Thread.currentThread().interrupt();
+            }
+            t = null;
+        }
+    }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
annot/src/main/java/com/predic8/membrane/annot/beanregistry/AsyncBeanCollector.java
around lines 31-42, the background thread lacks lifecycle controls and
resilience: add a public stop() method that interrupts the worker thread and
waits (join with a timeout) for it to terminate, set the thread to daemon
(t.setDaemon(true)) and give it a descriptive name
(t.setName("AsyncBeanCollector-worker")), and wrap delegate.handle(...) in a
try/catch(Throwable) that logs unexpected runtime errors and continues the loop
so the thread does not die silently; ensure InterruptedException handling breaks
the loop and preserves interrupt status as appropriate.

Comment on lines +113 to +114
for (String uid : uidsToRemove)
uidsToActivate.remove(uid);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Bug: Removal from uidsToActivate will never work.

uidsToActivate is a Set<UidAction>, but the removal loop attempts to remove String UIDs. Since UidAction is a record with (String uid, WatchAction action), calling uidsToActivate.remove(uid) where uid is a String will never match any UidAction element.

🔎 Proposed fix
-        for (String uid : uidsToRemove)
-            uidsToActivate.remove(uid);
+        uidsToActivate.removeIf(ua -> uidsToRemove.contains(ua.uid()));

Or alternatively, clear processed items inline:

     private void activationRun() {
-        Set<String> uidsToRemove = new HashSet<>();
+        Iterator<UidAction> it = uidsToActivate.iterator();
-        for (UidAction uidAction : uidsToActivate) {
+        while (it.hasNext()) {
+            UidAction uidAction = it.next();
             BeanContainer bc = bcs.get(uidAction.uid());
             try {
                 // ... existing code ...
-                uidsToRemove.add(bc.getDefinition().getUid());
+                it.remove();
             } catch (Exception e) {
                 // ... existing code ...
             }
         }
-        for (String uid : uidsToRemove)
-            uidsToActivate.remove(uid);
     }
🤖 Prompt for AI Agents
In
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
around lines 113-114, the loop calls uidsToActivate.remove(uid) using a String
uid while uidsToActivate is a Set<UidAction>, so removals never match; replace
the removal logic to remove the UidAction entries whose uid field equals the
String (for example, use uidsToActivate.removeIf(ua ->
uidsToRemove.contains(ua.uid())) or iterate with an Iterator and remove when
ua.uid().equals(uid)), or simply clear processed items inline after mapping,
ensuring type-correct removal of UidAction instances rather than attempting to
remove Strings.

@membrane-ci-server

Copy link
Copy Markdown

This pull request needs "/ok-to-test" from an authorized committer.

@christiangoerdes

Copy link
Copy Markdown
Collaborator Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants